CODENVY-560 : new types of docker recipes, remove InstanceKey, rework InstanceProvider #1366

benoitf · 2016-05-27T07:10:39Z

1. new docker recipe type

currently we have type:"dockerfile", location: "http://path-to-recipe"

now we could provide
type:"dockerfile", content: "FROM codenvy/foo\nENV FLORENT=TRUE"

and
type:"image", location : "codenvy/foo"

2. InstanceKey

Up to now, InstanceKey was used to perform snapshot recovery.
But machine source is a way to provide this information.
So remove InstanceKey and replace it by MachineSource (and DockerMachineSource instead of DockerInstanceKey)

InstanceProvider:
void removeInstanceSnapshot(InstanceKey instanceKey)
--> void removeInstanceSnapshot(MachineSource machineSource)

Instance:
InstanceKey saveToSnapshot(String owner)
--> MachineSource saveToSnapshot(String owner)

3. InstanceProvider model

To avoid also that MachineManager "knows" the inner type, the recipe handling is moved to the instance provider implementation
And as the snapshot handling is with MachineSource (included in MachineConfig included in Machine), no need to give extra InstanceKey parameter

Replace two previous methods

Instance createInstance(Recipe recipe,
Machine machine,
LineConsumer creationLogsOutput)

Instance createInstance(InstanceKey instanceKey,
Machine machine,
LineConsumer creationLogsOutput) throws NotFoundException, InvalidInstanceSnapshotException, MachineException;

by only one:
createInstance(Machine machine,
LineConsumer creationLogsOutput)

Change-Id: Ia7ea97bc1a44059b4892f5db387f54f2e1709fa3
Signed-off-by: Florent BENOIT fbenoit@codenvy.com

gazarenkov · 2016-05-27T07:31:34Z

ok with me

@skabashnyuk @garagatyi ?

garagatyi · 2016-05-27T08:30:03Z

This PR can't be merged, please rebase

garagatyi · 2016-05-27T08:33:30Z

core/che-core-api-model/src/main/java/org/eclipse/che/api/core/model/machine/MachineSource.java

+     * Alternate way is to provide a location
+     * @param content the content instead of an external link like with location
+     */
+    void setContent(String content);


We don't use setters in model interfaces

codenvy-ci · 2016-05-27T10:28:06Z

Build # 728 - FAILED

Please check console output at http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/728/ to view the results.

codenvy-ci · 2016-05-29T14:03:38Z

Build # 742 - FAILED

Please check console output at http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/742/ to view the results.

garagatyi · 2016-05-29T14:31:15Z

...cker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceProvider.java

-                                                                                         machine.getId(),
-                                                                                         userName,
-                                                                                         machine.getConfig().getName());
+    protected Instance doCreateInstanceImage(final Machine machine, String machineContainerName,


This method returns Instance which is representation of the container. Please rename this method.

It's creating instance for type image si method name is accurate

It looks to me as create instance of image instead of create instance from image. But if you don't want to change then ok.

createInstanceFromImage() ?

benoitf · 2016-05-29T19:07:54Z

are we good now ?

garagatyi · 2016-05-30T07:17:13Z

...cker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceProvider.java

        if (registry == null || repository == null) {
-            LOG.error("Failed to remove instance snapshot: invalid instance key: {}", instanceKey);
+            LOG.error("Failed to remove instance snapshot: invalid instance key: {}", dockerMachineSource);


It is not instance key anymore

garagatyi · 2016-05-30T08:03:43Z

...-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerMachineSource.java

+     * it's based on optional registry followed by repository name followed by a digest or a tag
+     * Examples are available in the test.
+     */
+    public static final Pattern IMAGE_PATTERN = Pattern.compile("^((?<registry>[^/]++)(?:\\/))?(?<repository>.*?)((?:\\:)(?<tag>.*?))?((?:@)(?<digest>.*))?$");


As far as I can see this regexp is not tested. Please review your tests.
Also it looks like
user/repo will be treated as registry user and repository repo

There is a current class DockerMachineSourceTest with REGEXP being tested by constructor calls
Also now it's delegating to DockerMachineIdentifierParser

garagatyi · 2016-05-30T11:16:38Z

LGTM

skabashnyuk · 2016-05-30T12:42:31Z

...t/src/main/java/org/eclipse/che/ide/extension/machine/client/machine/MachineManagerImpl.java

+     * Converts {@link MachineSource} to {@link MachineSourceDto}.
+     */
+    public MachineSourceDto asDto(MachineSource source) {
+        return this.dtoFactory.createDto(MachineSourceDto.class).withType(source.getType()).withLocation(source.getLocation()).withContent(source.getContent());


wrap lines? Would it be more readable?

skabashnyuk · 2016-05-30T12:44:08Z

ok

…ove InstanceKey #1 new docker recipe type currently we have type:"dockerfile", location: "http://path-to-recipe" now we could provide type:"dockerfile", content: "FROM codenvy/foo\nENV FLORENT=TRUE\" and type:"image", location or content: "codenvy/foo" #2 InstanceKey Up to now, InstanceKey was used to perform snapshot recovery. But machine source is a way to provide this information. So remove InstanceKey and replace it by MachineSource (and DockerMachineSource instead of DockerInstanceKey) InstanceProvider: void removeInstanceSnapshot(InstanceKey instanceKey) --> void removeInstanceSnapshot(MachineSource machineSource) Instance: InstanceKey saveToSnapshot(String owner) --> MachineSource saveToSnapshot(String owner) #3 InstanceProvider model To avoid also that MachineManager "knows" the inner type, the recipe handling is moved to the instance provider implementation And as the snapshot handling is with MachineSource (included in MachineConfig included in Machine), no need to give extra InstanceKey parameter Replace two previous methods Instance createInstance(Recipe recipe, Machine machine, LineConsumer creationLogsOutput) Instance createInstance(InstanceKey instanceKey, Machine machine, LineConsumer creationLogsOutput) throws NotFoundException, InvalidInstanceSnapshotException, MachineException; by only one: createInstance(Machine machine, LineConsumer creationLogsOutput) Change-Id: Ia7ea97bc1a44059b4892f5db387f54f2e1709fa3 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

codenvy-ci · 2016-05-30T15:13:13Z

Build # 752 - FAILED

Please check console output at http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/752/ to view the results.

garagatyi reviewed May 27, 2016
View reviewed changes

TylerJewell mentioned this pull request May 28, 2016

creating containers from local dockerfiles #1379

Closed

benoitf force-pushed the CODENVY-560 branch from c07fa5a to 82b3ffa Compare May 29, 2016 13:06

benoitf mentioned this pull request May 29, 2016

CODENVY-560 Adapt changes to new management of MachineSource in Che codenvy/codenvy#238

Merged

garagatyi reviewed May 29, 2016
View reviewed changes

garagatyi reviewed May 30, 2016
View reviewed changes

benoitf force-pushed the CODENVY-560 branch from 82b3ffa to 11a9f61 Compare May 30, 2016 11:02

benoitf force-pushed the CODENVY-560 branch from 11a9f61 to a2b5698 Compare May 30, 2016 11:40

skabashnyuk reviewed May 30, 2016
View reviewed changes

benoitf force-pushed the CODENVY-560 branch from 25b52bd to ae4c552 Compare May 30, 2016 12:51

benoitf merged commit ae4c552 into master May 30, 2016

benoitf deleted the CODENVY-560 branch May 30, 2016 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CODENVY-560 : new types of docker recipes, remove InstanceKey, rework InstanceProvider #1366

CODENVY-560 : new types of docker recipes, remove InstanceKey, rework InstanceProvider #1366

benoitf commented May 27, 2016 •

edited

Loading

gazarenkov commented May 27, 2016

garagatyi commented May 27, 2016

garagatyi May 27, 2016

benoitf May 27, 2016

benoitf May 29, 2016

codenvy-ci commented May 27, 2016

codenvy-ci commented May 29, 2016

garagatyi May 29, 2016

benoitf May 29, 2016

garagatyi May 30, 2016

benoitf May 30, 2016

garagatyi May 30, 2016

benoitf May 30, 2016

garagatyi May 30, 2016

benoitf commented May 29, 2016

garagatyi May 30, 2016

benoitf May 30, 2016

benoitf May 30, 2016

garagatyi May 30, 2016

benoitf May 30, 2016

garagatyi May 30, 2016

garagatyi commented May 30, 2016

skabashnyuk May 30, 2016

benoitf May 30, 2016

skabashnyuk commented May 30, 2016

codenvy-ci commented May 30, 2016

CODENVY-560 : new types of docker recipes, remove InstanceKey, rework InstanceProvider #1366

CODENVY-560 : new types of docker recipes, remove InstanceKey, rework InstanceProvider #1366

Conversation

benoitf commented May 27, 2016 • edited Loading

1. new docker recipe type

2. InstanceKey

3. InstanceProvider model

gazarenkov commented May 27, 2016

garagatyi commented May 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented May 27, 2016

codenvy-ci commented May 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf commented May 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi commented May 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skabashnyuk commented May 30, 2016

codenvy-ci commented May 30, 2016

benoitf commented May 27, 2016 •

edited

Loading